Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Qt] add verifySize() function to PaymentServer #5665

Merged
merged 1 commit into from Sep 15, 2015
Merged

[Qt] add verifySize() function to PaymentServer #5665

merged 1 commit into from Sep 15, 2015

Conversation

Diapolo
Copy link

@Diapolo Diapolo commented Jan 15, 2015

  • add static verifySize() function to PaymentServer and move the logging
    on error into the function
  • the function checks if the size is allowed as per BIP70

@laanwj laanwj added the GUI label Jan 15, 2015
@jonasschnelli
Copy link
Contributor

I would also strongly recommend to add a Unit-Test for verifySize().
We should no longer add implementations without test-coverage.
And i assume this is easy to test with another payment request in the fixtures.

@Diapolo
Copy link
Author

Diapolo commented Jan 30, 2015

@jonasschnelli As verifySize() IS used in PaymentServer::readPaymentRequestFromFile() we already have test coverage :). IMHO it doesn't make sense to spam paymentrequestdata.h with 50001 Bytes of garbage just to re-test verifySize() again. What do you think?

@Diapolo Diapolo mentioned this pull request Jan 30, 2015
Merged
@jonasschnelli
Copy link
Contributor

@Diapolo IMO we should add the 50001 bytes test. You might use this forged payment request http://jonasschnelli.ch/r1422608371.bitcoinpaymentrequest

I also tried to test PaymentServer::readPaymentRequestFromFile with 50001 random bytes. But there is no distinction between parsing error and size error (returns only a bool).
Is there a change to test the QWarning within a QT unit test? Just a little string comparison?

@Diapolo
Copy link
Author

Diapolo commented Jan 30, 2015

@jonasschnelli I'm fine with directly using verifySize() instead of PaymentServer::readPaymentRequestFromFile() (which can then be private again ^^). But we don't need a real invalid playment request for this check IMHO. As the check is triggerd by just if > 50000 bytes rule it can be random garbage data.

What do you mean by Is there a change to test the QWarning within a QT unit test? Just a little string comparison??

@jonasschnelli
Copy link
Contributor

Tested, reviewed, stepped ACK.

nit: misses unit test

bildschirmfoto 2015-01-30 um 10 10 42

@jonasschnelli
Copy link
Contributor

@Diapolo: sounds even better. I would recommend to just pass 50001 bytes through PaymentServer::verifySize. I understand that the test looks very trivial know. But the idea of test are that they give guidelines when extending the implementation.

@Diapolo
Copy link
Author

Diapolo commented Jan 30, 2015

@jonasschnelli Updated, I hope this is fine now?

@jonasschnelli
Copy link
Contributor

tested ACK

@Diapolo
Copy link
Author

Diapolo commented Feb 3, 2015

Anything left to do for me to get this and other related pulls merged ;)?

@Diapolo
Copy link
Author

Diapolo commented Feb 26, 2015

Ping

@Diapolo
Copy link
Author

Diapolo commented Mar 9, 2015

@laanwj Mind giving this the final ACK ;)?

@jonasschnelli
Copy link
Contributor

ReACK

@Diapolo
Copy link
Author

Diapolo commented Mar 21, 2015

@laanwj ping

laanwj
laanwj reviewed Mar 24, 2015
View reviewed changes
src/qt/test/paymentservertests.cpp
@@ -185,7 +185,8 @@ void PaymentServerTests::paymentServerTests()
tempFile.open();
tempFile.write((const char*)randData, sizeof(randData));
tempFile.close();
QCOMPARE(PaymentServer::readPaymentRequestFromFile(tempFile.fileName(), r.paymentRequest), false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you removing this test?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be able to make readPaymentRequestFromFile private again, which I changed once and wasn't happy with it :). The function could also fail, if the file failed to open, which isn't what this test is for anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to be able to test a private methods you could use a friend class / function. E.g. inside the class do

friend class PaymentServerTester;

Then do the testing in a class PaymentServerTester.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@laanwj It's not necessary here, but thanks for that C++ lesson :).

@Diapolo
Copy link
Author

Diapolo commented Apr 12, 2015

@jonasschnelli Maybe you can help me understand what's preventing this pull from getting merged?

@Diapolo
Copy link
Author

Diapolo commented May 31, 2015

@laanwj @jonasschnelli Ping

@laanwj
Copy link
Member

laanwj commented Jun 4, 2015

@Diapolo because you removed the test for, unrelated, PaymentServer::readPaymentRequestFromFile

@Diapolo
Copy link
Author

Diapolo commented Jun 5, 2015

@laanwj As I said, I just made PaymentServer::readPaymentRequestFromFile() public, because I hacked the former unit-test. That function isn't reliable as unit-test because it can also fail if the file could not be opened (returns false then), which isn't what the test assumes, as we want to verify the DoS protection.

@jgarzik
Copy link
Contributor

jgarzik commented Jul 23, 2015

ut ACK

@Diapolo
Copy link
Author

Diapolo commented Jul 24, 2015

@laanwj Fixed merge-conflict, ready now.

[Qt] add verifySize() function to PaymentServer
be942de 
- add static verifySize() function to PaymentServer and move the logging
  on error into the function
- also use the new function in the unit test
- the function checks if the size is allowed as per BIP70
@Diapolo
Copy link
Author

Diapolo commented Aug 10, 2015

@laanwj Ping

@Diapolo
Copy link
Author

Diapolo commented Sep 9, 2015

Final ping!

@jgarzik jgarzik merged commit be942de into bitcoin:master Sep 15, 2015
jgarzik pushed a commit that referenced this pull request Sep 15, 2015
Merge pull request #5665
6f55cdd
@Diapolo Diapolo deleted the pr_size branch September 15, 2015 21:04
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
GUI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Diapolo @jonasschnelli @laanwj @jgarzik